Skip to content

Migrate derivative to educe #8125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 17 commits into
base: unstable
Choose a base branch
from
Open

Migrate derivative to educe #8125

wants to merge 17 commits into from

Conversation

jchavarri
Copy link

@jchavarri jchavarri commented Sep 27, 2025 •
edited
Loading

Issue Addressed

Fixes #7001.

Proposed Changes

Mostly mechanical replacement of derivative attributes with educe ones.

Attribute Syntax Changes

// Bounds: = "..." → (...)
#[derivative(Hash(bound = "E: EthSpec"))]
#[educe(Hash(bound(E: EthSpec)))]

// Ignore: = "ignore" → (ignore)  
#[derivative(PartialEq = "ignore")]
#[educe(PartialEq(ignore))]

// Default values: value = "..." → expression = ...
#[derivative(Default(value = "ForkName::Base"))]
#[educe(Default(expression = ForkName::Base))]

// Methods: format_with/compare_with = "..." → method(...)
#[derivative(Debug(format_with = "fmt_peer_set_as_len"))]
#[educe(Debug(method(fmt_peer_set_as_len)))]

// Empty bounds: removed entirely, educe can infer appropriate bounds
#[derivative(Default(bound = ""))]
#[educe(Default)]

// Transparent debug: manual implementation (educe doesn't support it)
#[derivative(Debug = "transparent")]
// Replaced with manual Debug impl that delegates to inner field

Note: Some bounds use strings (bound("E: EthSpec")) for superstruct compatibility (expected ',' errors).

Additional Info

derivate remains as a transitive dep through alloy-primitives -> ruint -> ark-ff -> derivative.

@jchavarri jchavarri requested a review from jxs as a code owner September 27, 2025 17:35
@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Sep 27, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Sep 27, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jchavarri
Copy link
Author

Note: Some bounds use strings (bound("E: EthSpec")) for superstruct compatibility (expected ',' errors).

It seems this was fixed in recent versions of darling: https://github.com/TedDriggs/darling/releases/tag/v0.20.3. Updating superstruct to 0.10.0 solves the limitation.

Aside from that, trying to figure out / reproduce locally the ci build errors on the check_consensus_block_proposals task.

@michaelsproul michaelsproul added code-quality dependencies Pull requests that update a dependency file labels Sep 28, 2025
@macladson macladson mentioned this pull request Sep 29, 2025
Merged
macladson
macladson reviewed Sep 29, 2025
View reviewed changes
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but for consistency I think we may as well bump superstruct so we can get rid of the strings in some of the bounds. I've opened a PR here -> #8133

mergify bot pushed a commit that referenced this pull request Sep 30, 2025
@macladson
Bump superstruct to 0.10.0 (#8133)
af5cbfb 
Bump `superstruct` to the latest release `0.10.0`.
This version uses a later version of `darling` which is helpful for #8125


Co-Authored-By: Mac L <[email protected]>
@macladson macladson removed the blocked label Sep 30, 2025
macladson
macladson requested changes Sep 30, 2025
View reviewed changes
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchavarri We have bumped the unstable branch to superstruct version 0.10.0
Can you change the trait bounds to remove those strings?

@macladson macladson added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Sep 30, 2025
@jchavarri
Copy link
Author

@macladson done! seems to work fine 🎉

@macladson macladson removed the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Sep 30, 2025
@macladson macladson added the ready-for-review The code is ready for review label Sep 30, 2025
macladson
macladson requested changes Sep 30, 2025
View reviewed changes
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me! Just some minor comments

@macladson macladson added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Sep 30, 2025
@macladson macladson added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 30, 2025
macladson
macladson approved these changes Oct 7, 2025
View reviewed changes
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some internal discussion I've reverted the Default impl change since it's cleaner and is irrelevant for the tests. Sorry for the mix-up.

This PR looks good now so we can merge soon 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macladson macladson macladson approved these changes

@jxs jxs Awaiting requested review from jxs jxs is a code owner

Assignees
No one assigned
Labels
code-quality dependencies Pull requests that update a dependency file ready-for-review The code is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jchavarri @macladson @michaelsproul